Skip to content

feat(install): CBM_PACKAGE_MANAGED guard + FreeBSD self-path detection - #1331

Closed
ocochard wants to merge 1 commit into
DeusData:mainfrom
ocochard:main
Closed

feat(install): CBM_PACKAGE_MANAGED guard + FreeBSD self-path detection#1331
ocochard wants to merge 1 commit into
DeusData:mainfrom
ocochard:main

Conversation

@ocochard

@ocochard ocochard commented Jul 29, 2026

Copy link
Copy Markdown

Problems

When installed by a package manager (FreeBSD ports, Homebrew, distro pkg), codebase-memory-mcp install misbehaves in two ways:

  1. Mutates $HOME — copies the running binary into ~/.local/bin (a second, unmanaged copy of a file the package manager already owns under its prefix) and appends export PATH="~/.local/bin:$PATH" to the user's shell rc. Both are redundant: the binary is already on PATH via the manager's prefix.

  2. Writes the wrong binary path into agent configs on FreeBSD. cbm_detect_self_path() used readlink("/proc/self/exe"), but FreeBSD has no /proc mounted by default, so detection failed and silently fell back to ~/.local/bin/codebase-memory-mcp. That bogus path was then written as the MCP command into .claude.json / .mcp.json, so the agent could never launch the server.

Fix

Add a compile-time guard CBM_PACKAGE_MANAGED (off by default; follows the existing -DCBM_* build-flag convention). When set, install:

  • skips the ~/.local/bin binary copy (do_copy = false),
  • skips the PATH step (no shell-rc edit; completion message drops the source <rc> hint), and
  • points bin_target — hence the MCP command written into every agent config — at the running executable instead of the hardcoded ~/.local/bin path.

Separately (all platforms, not gated): add a FreeBSD branch to cbm_detect_self_path() using sysctl KERN_PROC_PATHNAME, which resolves the running executable without /proc. Non-FreeBSD detection is unchanged.

Verification

Built on FreeBSD with -DCBM_PACKAGE_MANAGED (via the devel/codebase-memory-mcp port, clean poudriere jail, -Werror, no warnings). Running codebase-memory-mcp install against a seeded Claude Code config now writes:

{ "mcpServers": { "codebase-memory-mcp": { "command": "/usr/local/bin/codebase-memory-mcp" } } }

No ~/.local/bin path anywhere, no shell rc touched. Source installs (scripts/build.sh, flag off) are unaffected.

@ocochard
ocochard requested a review from DeusData as a code owner July 29, 2026 06:03
…path

When built with -DCBM_PACKAGE_MANAGED, `install` no longer mutates the
user's $HOME and wires agent configs to the real installed binary:

- skips copying the running binary into ~/.local/bin (the package manager
  already owns it under its prefix), and
- skips appending `export PATH=...` to the shell rc, and
- points bin_target (hence the MCP `command` written into .claude.json,
  .mcp.json, etc.) at the running executable instead of the hardcoded
  ~/.local/bin path.

Also add a FreeBSD self-path branch: cbm_detect_self_path() used
readlink("/proc/self/exe"), but FreeBSD has no /proc by default, so it
silently fell back to ~/.local/bin — writing that wrong path into every
agent config. Use sysctl KERN_PROC_PATHNAME, which needs no /proc.

The flag is off by default; source installs (scripts/build.sh) are
unchanged. Non-FreeBSD self-path detection is unchanged.
@ocochard ocochard changed the title feat(install): add CBM_PACKAGE_MANAGED build guard feat(install): CBM_PACKAGE_MANAGED guard + FreeBSD self-path detection Jul 29, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thank you for this — the diagnosis is correct and the flag design is genuinely careful. Three things: one you can fix now, one that is not your fault, and one that needs the maintainer.

Your FreeBSD diagnosis is exactly right. FreeBSD mounts no /proc by default, so the existing readlink("/proc/self/exe") silently failed and a bogus ~/.local/bin path got written into .claude.json and .mcp.json. sysctl(KERN_PROC_PATHNAME) with pid = -1 is the correct fix, and thank you for verifying it in a clean poudriere jail with -Werror and showing the resulting config.

The flag design is genuinely additive, and I verified that rather than taking it on trust. With CBM_PACKAGE_MANAGED unset, the only delta is capturing a previously-discarded return value and (void)-ing it — byte-for-byte identical behaviour for every existing user on every shipped platform. The FreeBSD branch is #elif defined(__FreeBSD__) inside the existing chain, so it is preprocessor-inert on Linux, macOS and Windows. I also confirmed the empty-shell_rc mechanism correctly short-circuits the PATH step in cli_install_activate.

One thing to fix now: the DCO check is a genuine failure. Commit dfd2ab22 has no Signed-off-by trailer matching its author:

git commit --amend -s
git push --force-with-lease

The sign-off email needs to match your commit author email. That alone blocks the merge regardless of everything else.

One failure that is not yours: test / test-unix (macos-15-intel) failed on lock_registry — a known transient-window flake on our slowest runner that asserts a 500 ms observation window. An install-command diff cannot touch lock registry code. I have flagged those tests separately for a determinism fix.

One defect worth fixing while you are in there. Under CBM_PACKAGE_MANAGED, if self-path detection fails, bin_target silently stays ~/.local/bin/... while the copy step is skipped — so the agent configs would point at a binary that was never installed there. That is a broken config written confidently. It should fail loudly instead.

Minor style note: #include <sys/sysctl.h> before <sys/types.h> — FreeBSD convention is types first, and it only builds because cli.c already pulls types in earlier.

What needs the maintainer, and why I am not deciding it: CBM_PACKAGE_MANAGED would be a permanent build-flag surface. Once ports or Homebrew build with it, we support it indefinitely. And the FreeBSD path raises the same question we have open on #1342 (NetBSD): do we accept code for a platform we cannot build, test, or regression-guard in CI? Both deserve one consistent answer, so I have put them together rather than answering one by accident.

Relatedly, and worth knowing: the CBM_PACKAGE_MANAGED path is compiled by no CI leg today, so all three guarded behaviours would be untested from CI's perspective. If it is accepted, a Linux compile leg with -DCBM_PACKAGE_MANAGED would be the minimum guard.

The work itself is tidy and the packaging motivation is legitimate — this is a policy question, not a quality one.

@DeusData DeusData added enhancement New feature or request editor/integration Editor compatibility and CLI integration ux/behavior Display bugs, docs, adoption UX labels Aug 3, 2026
@DeusData DeusData added this to the 0.9.2-rc milestone Aug 3, 2026
@DeusData DeusData added the priority/backlog Valuable contribution, lower scheduling urgency; review when maintainer capacity opens. label Aug 3, 2026
@DeusData

DeusData commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Thank you for this PR — the problem is real and well worth fixing: self-update clobbering a package-manager-owned binary and editing the user's shell rc is exactly the wrong behavior for a ports/package install, and FreeBSD reaching cbm through ports would be a genuinely nice distribution win. The sysctl(KERN_PROC_PATHNAME) self-path detection is also the right primitive there.

Before we can take it, though, our review turned up a few functional gaps we'd like your read on:

  1. macOS/Homebrew defeats the guard: once bin_target is repointed at the running executable, sign_binary = do_copy || target_exists is always true — so the install path still stages, ad-hoc-signs, and re-commits onto the package-manager-owned binary in its own prefix (or hard-fails on a root-owned prefix). That's the exact clobber the flag is meant to prevent, on one of the platforms a packager would build for.
  2. Detection-failure fallback reintroduces the bug: with do_copy forced false but bin_target only repointed if (self_path_exact), a failed self-path detection means nothing is copied yet a bogus ~/.local/bin path is still written into the agent configs — silently.
  3. Partial suppression: bin_dir is still created (~/.local/bin), the Windows PATH edit isn't gated (the suppression is POSIX-only), and --dir gets silently ignored under the guard.
  4. Testability: as a compile-time macro that no build we gate ever defines, the branch is never compiled, linted, or tested by any CI leg on any OS — and several existing --dir install tests would fail under the flag as written.

Our main question is about the layer: would a runtime decision work for your use case — e.g. "the running binary is not under bin_dir (or bin_dir isn't ours) → don't copy, don't touch rc, point configs at the running exe"? That reaches the same outcome for ports, needs no build variant, keeps one binary behavior everywhere, and is testable by the existing suite. If there's a packaging constraint that genuinely needs the compile-time gate (e.g. ports policy about runtime heuristics), we'd like to understand it before deciding the direction.

Also fyi: resolve_self_executable() in src/ui/http_server.c:908 still uses readlink("/proc/self/exe"), so the FreeBSD path fix would want to cover that call site too.

No action needed on the defects yet — we'd like your feedback on the runtime-vs-compile-time question first, then we can converge on the shape. Thanks again for bringing FreeBSD packaging to the table!

@ocochard

ocochard commented Aug 5, 2026

Copy link
Copy Markdown
Author

Ran the runtime approach as an actual FreeBSD port, it works, with one caveat worth your call.

Runtime detection is the right answer for the install-path behavior. I replaced the compile-time gate with a runtime check: if self_path (OS-reported, not the fallback) is not under bin_dir, the binary is package-owned → don't copy, don't sign, don't touch rc, point configs at the running exe. Verified end-to-end from /usr/local/bin: no copy into ~/.local/bin, shell rc untouched, agent config → the real binary path. This also fixes both defects you flagged: the macOS re-sign (guarded by !package_managed) and the detection-failure fallback (the check requires the OS-reported path, so a failed detection never enters package-managed mode or writes an unverified path).

But CBM_PACKAGE_MANAGED was gating two things, not one. Besides install-path, it also disables the update subcommand and the background GitHub update-check. Those are lifecycle, not location, a runtime "am I under bin_dir" check can't express "this binary must never self-update." So I'd propose a hybrid: install-path goes fully runtime (no flag), and the flag survives only to suppress self-update for package builds. That shrinks the build surface to one narrow, defensible purpose instead of removing it.

Does that split work for you? If so I'll rework the PR to that shape (and cover the http_server.c:908 call site + include order + DCO in the same pass).

@DeusData

Copy link
Copy Markdown
Owner

Answering your "does that split work for you?" — and the honest answer is no split is needed, because main now does both halves at runtime. But the reason I am not just closing this is that your actual bug is still there, and I verified it rather than assuming.

Sorry for the fifteen days. You did the empirical work I asked for and then waited.

What landed while this sat.

  • Your FreeBSD self-path fix is on main — commit 305b89bf, 28 July, the same KERN_PROC_PATHNAME sysctl. That is one day before you opened this PR, which means you were fixing something that had just been fixed and neither of us noticed. That is a review-queue failure, not yours.
  • The package-manager install behaviour is on main too — commit 0e89ef87 (13 August), implemented as exactly the runtime inference I proposed on 4 August: cli_binary_is_externally_managed(), --skip-binary / --force-binary overrides, PATH touched only when we placed the binary.
  • And your lifecycle objection is already answered. You argued that a runtime "am I under bin_dir" test cannot express "this binary must never self-update", so the build flag had to survive for that. Fair argument — but main's update now checks cli_binary_is_externally_managed() directly and refuses on a foreign binary. So self-update suppression is a runtime property after all, and there is nothing left for CBM_PACKAGE_MANAGED to gate.

(Small note: the update-gating you described is not in this PR's diff — it has three CBM_PACKAGE_MANAGED sites and none touch update. I assume you were describing your downstream port patch rather than this branch.)

Now the part that matters: a FreeBSD ports install is still broken on main today.

cli_external_manager_name() deliberately uses positive evidence only — it matches /mise/, /.mise/, /Cellar/, /homebrew/, /linuxbrew/, /nix/store/, /.asdf/ and /.cargo/bin/. The comment explains why the tempting rule was rejected: "anything outside the directory we install into" misreads a test binary running from build/ and misreads a perfectly ordinary install --dir=/opt/cbm.

/usr/local/bin is not in that list. So a binary installed by FreeBSD ports is not recognised as externally managed, and install still copies it into ~/.local/bin, still appends to your shell rc, and still points agent configs at the copy — the exact behaviour this PR set out to stop. Your use case is unfixed; only the mechanism changed underneath you.

So there is real work left, and it is smaller and better-shaped than the flag. Two pieces, and I would rather you owned them than I guessed:

  1. Teach cli_external_manager_name() about system package prefixes. This needs your FreeBSD knowledge, because the naive entry is wrong: /usr/local/bin is the ports prefix on FreeBSD, but it is also Homebrew's prefix on Intel macOS and a perfectly legitimate --dir target anywhere. A platform-conditional match — treat /usr/local/bin/ as package-managed on FreeBSD specifically — looks right to me, with --force-binary as the escape hatch for someone who deliberately installed there. Does that match how ports actually behave? Is /usr/local/bin the only prefix worth matching, or should the check be broader?
  2. resolve_self_executable() in src/ui/http_server.c still has no FreeBSD branch. It is __APPLE__ or readlink("/proc/self/exe"), so on a FreeBSD box without /proc mounted it simply returns false. Same sysctl call as your cli.c fix. I flagged this on 4 August and it is still open.

I am closing this PR, because its diff is now entirely superseded and it conflicts against a cli.c that has been substantially rewritten — reworking in place would be harder than starting from current main. That is not a rejection of the work. If you open a fresh PR with those two pieces I will review it promptly, and this time I will not let it sit.

One housekeeping item for whenever you do: the DCO check failed here because the commit carries no matching Signed-off-by. git commit --amend -s fixes it.

Thank you for the FreeBSD expertise, and particularly for taking the runtime approach seriously and going to test it on a real port instead of arguing the point. That is what made the direction call easy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

editor/integration Editor compatibility and CLI integration enhancement New feature or request priority/backlog Valuable contribution, lower scheduling urgency; review when maintainer capacity opens. ux/behavior Display bugs, docs, adoption UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants